Thank you @tjni!

It seems strange that make_server would be allowed to raise an error without causing the construction of the object to fail, though. What circumstances did you see this happen in? Would you be able to add a test case? Or at least share a bit of sample code or an explanation of how it happens which we could use to make a test case? This is something I would definitely like to have tested as part of the PR.

Thank you for the fast review. Would it be all right to add a test that mocks make_server? The best I think I can try to do is raise an error in the mock and verify that no warning is emitted to stderr (since that is what happens when an exception is raised in __del__ [reference]).

Indeed the construction of the object fails when make_server raises an error, but __del__ is called anyway. In my specific circumstance, I was running an unrelated build in a sandbox that prevented connecting to localhost, so a server could not be started successfully.

Ah, I see, thanks. I had forgotten about stop() getting called from __del__().

I think mocking make_server() makes perfect sense here. The whole point of your change is to handle the case where that function raises an exception, and for the test there's no need for the function to do anything else other than raising the exception.

I tried to write a test and realized why it's hard. Any suggestions welcome!

def test_httpserver_init_failure_no_warning_during_cleanup(capfd):
    with patch("pytest_localserver.http.make_server", side_effect=Exception("init failure")):
        try:
            server = http.ContentServer()
        except Exception:
            pass

    assert capfd.readouterr().err == ""

This unfortunately doesn't work because we cannot guarantee that the finalizer runs by the time we assert. When I test locally, the finalizer only runs after all the tests finish.

Hmm yeah I see what you mean. Off the top of my head, what about putting the server-manipulation code in a helper function? Maybe stack unwinding will trigger the finalizer to run.

def fail_to_create_server():
    with patch("pytest_localserver.http.make_server", side_effect=Exception("init failure")):
        # Call this just to trigger an exception during construction of the ContentServer
        server = http.ContentServer()

def test_httpserver_init_failure_no_warning_during_cleanup(capfd):
    try:
        fail_to_create_server()
    except Exception:
        pass

    assert capfd.readouterr().err == ""

If that doesn't work, you could try running gc.collect() in a finally block inside the function, although that does seem rather heavy-handed. 🤷

I'll return to this issue tomorrow or later this week and let you know if I think of anything else.

Oh BTW I'd probably suggest using a more specific error type when mocking - maybe RuntimeError().

And that patch context manager you're using, that's not part of pytest, is it? If not, you could use pytest.monkeypatch to avoid introducing a new dependency, although admittedly monkeypatch doesn't have as clean syntax... I could see going either way on that but I'm slightly leaning toward not introducing a new dependency just for a small piece of functionality like that.

Thank you! I will also take a closer look tomorrow morning and try out your suggestions.

patch comes from unittest.mock, which is available from Python 3.3+.

Ahh, gotcha. In that case it should be fine. Personally I've tried to avoid mixing different test frameworks (i.e. pytest and unittest) just to keep the code easier to understand, but it's not really hurting much to do so, and if it results in cleaner code to use something from unittest then that's a win.

I tried several ways to force __del__() to run and detect the resulting error / warning, but I was not successful. Things I tried included combinations of:

1. Creating a helper function to create the server.
2. Calling gc.collect().
3. Capturing the warning using pytest.warns().
4. Sleeping for a second.

My current approach is to try to do the next best thing, which is to simulate the scenario by deleting the _server field of a successfully constructed ContentServer.

Hmm, okay, well if necessary we can work with that.

Is the code where you originally encountered this error something that you can share? Or, could you make an equivalent example that you'd be able to share? Even if it's something complex, I think we might be able to distill it down into something that will reproduce the error - I'd at least like to try, if possible.

Great idea! If you have a macOS system (or runner), here's how you can reproduce it.

Start in an environment with pytest-localserver installed and the following script test.py:

from pytest_localserver import http

server = http.ContentServer()

server.start()
server.stop()

You can verify that running:

python test.py

works just fine and exits with status code 0.

However, running the following does not:

> sandbox-exec -p "(version 1) (allow default) (deny network*)" python test.py
Operation not permitted
Exception ignored in: <function WSGIServer.__del__ at 0x1043b6670>
Traceback (most recent call last):
  File "/Users/tni/code/pytest-localserver-test-cause/env/lib/python3.9/site-packages/pytest_localserver/http.py", line 31, in __del__
    self.stop()
  File "/Users/tni/code/pytest-localserver-test-cause/env/lib/python3.9/site-packages/pytest_localserver/http.py", line 34, in stop
    self._server.shutdown()
AttributeError: 'ContentServer' object has no attribute '_server'

Fantastic, thanks! I actually don't have a Mac, but I managed to reproduce the failure by just wrapping the whole body of the script in unittest.mock.patch like you were doing in the test case.

from pytest_localserver import http
from unittest.mock import patch

with patch("pytest_localserver.http.make_server", side_effect=Exception("init failure")):
    server = http.ContentServer()

    server.start()
    server.stop()

I think it would be totally fine to just run that in a test as a subprocess. I mean, I'll keep looking for a way to do it as an in-process test with pytest, and feel free to do the same, but a test with a separate script run as a subprocess is a good "checkpoint" and enough to proceed with the PR as far as I'm concerned.

The hack of deleting the _server attribute is quite reasonable as well, but the thing I like about using the separate script is that it gives a little more context about why this sort of thing might happen in practice, and that could be useful information for anyone who comes along to work on this in the future.

I rewrote the test based on your suggestion, using a subprocess. Please take a look :)

Looks good so far 😄 Just double-checking, should I go ahead and merge this once the workflow runs complete, assuming they all pass? Or did you want to do anything else with it first? (I don't know if Github will let you merge it yourself since I've approved the changes, but if it will, feel free to do so if you want)

Feel free to merge it, thank you for all of your help.

And thank you for the fix!